Skip to content

Comments

dont put haplotypes in ReferencePathOverlays#236

Merged
glennhickey merged 2 commits intomasterfrom
fix-ref-overlay
Feb 6, 2026
Merged

dont put haplotypes in ReferencePathOverlays#236
glennhickey merged 2 commits intomasterfrom
fix-ref-overlay

Conversation

@glennhickey
Copy link
Contributor

Let's keep ReferencePathOverlays for Reference Paths. Otherwise, memory can get out of hand on larger graphs...

Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea of "hidden" paths needs to be removed, since they don't exist anymore (and that's what was causing the over-indexing). But it looks like this will fix the over-indexing.

Comment on lines +49 to +52
/// even if they are hidden.
ReferencePathOverlay(const PathHandleGraph* graph, bool all_paths = false);

/// Same, but also indexes hidden paths listed in extra_path_names.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't/shouldn't have a concept of a path being "hidden" anymore.

Comment on lines +54 to +55
const std::unordered_set<std::string>& extra_path_names,
bool all_paths = false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all_paths is true, extra_path_names can't really be used, because we'll do all paths, since none of them are hidden anymore.

Comment on lines -22 to -25
// TODO: If we made the overlay transparent so we could access paths
// that didn't get indexed, we wouldn't be weirdly indexing haplotype
// paths from backends that don't hide them in the "reference" path
// overlay.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now all the backends are like that; I think I forgot to address that here when I made that change.

@glennhickey glennhickey merged commit e3ac0f4 into master Feb 6, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants